Use match statement in checkers (2) #10528
Conversation
cdce8p
added
Maintenance
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (98.84%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10528 +/- ##
==========================================
+ Coverage 95.84% 95.88% +0.03%
==========================================
Files 177 177
Lines 19289 19346 +57
==========================================
+ Hits 18488 18549 +61
+ Misses 801 797 -4
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
cdce8p
force-pushed
the
match-explore-2
branch
from
08d0a10 to
b444fff
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Pierre Sassoulas <[email protected]>
cdce8p
force-pushed
the
match-explore-2
branch
from
b444fff to
933a16c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| continue | ||
| else: | ||
| match inferred := checker_utils.safe_infer(ctx_mgr): | ||
| case _ if not inferred: |
There was a problem hiding this comment.
We're missing the case where isinstance(inferred, util.UninferableBase) that was previously handled ?
There was a problem hiding this comment.
While reviewing the checker code base I've seen the not inferred check in two different ways.
if inferred is None or isinstance(inferred, util.UninferableBase): ...
if not inferred: ...Those could be translated to
match inferred:
case None | util.UninferableBase():
...
case _ if not inferred:
...The later works since bool(util.UninferableBase()) also evaluates to False.
https://github.com/pylint-dev/astroid/blob/v4.0.0b2/astroid/util.py#L42-L43
--
I decided it might be better to just use one of the styles going forward. My slight preference would be towards case _ if not inferred.
| case nodes.Assign( | ||
| targets=[nodes.AssignName(), *_], value=nodes.Lambda() as value |
There was a problem hiding this comment.
node is always going to be a nodes.Assign, shouldn't we match node.targets[0] ?
There was a problem hiding this comment.
We also need to match node.value. Technically this introduces one redundant isinstance check however I think the readability improvement makes up for that.
I've also read somewhere that they wanted to cache the isinstance check results but haven't checked if the CPython type actually implemented it.
--
In theory we could also replace nodes.Assign(...) with just object(...). That would work two though I don't think it would be particularly better.
| msg = "non-ascii-name" | ||
|
|
||
| # Some node types have customized messages | ||
| if node_type == "file": | ||
| msg = "non-ascii-file-name" | ||
| elif node_type == "module": | ||
| msg = "non-ascii-module-import" | ||
| match node_type: | ||
| case "file": | ||
| msg = "non-ascii-file-name" | ||
| case "module": | ||
| msg = "non-ascii-module-import" | ||
| case _: | ||
| msg = "non-ascii-name" |
| if ( | ||
| isinstance(func, astroid.BoundMethod) | ||
| and isinstance(func.bound, astroid.Instance) | ||
| and func.bound.name in {"str", "unicode", "bytes"} | ||
| ): | ||
| if func.name in {"strip", "lstrip", "rstrip"} and node.args: | ||
| arg = utils.safe_infer(node.args[0]) | ||
| if not isinstance(arg, nodes.Const) or not isinstance(arg.value, str): | ||
| return | ||
| if len(arg.value) != len(set(arg.value)): | ||
| self.add_message( | ||
| "bad-str-strip-call", | ||
| node=node, | ||
| args=(func.bound.name, func.name), | ||
| ) | ||
| elif func.name == "format": | ||
| self._check_new_format(node, func) | ||
| match func := utils.safe_infer(node.func): | ||
| case astroid.BoundMethod( | ||
| bound=astroid.Instance(name="str" | "unicode" | "bytes" as bound_name), | ||
| ): |
There was a problem hiding this comment.
The match case really shine in those cases.
There was a problem hiding this comment.
I completely agree. There will be more, many more 😉 (#10531)
| if isinstance(default1, nodes.Const) and isinstance(default2, nodes.Const): | ||
| if default1.value != default2.value: | ||
| return True | ||
| elif isinstance(default1, nodes.Name) and isinstance(default2, nodes.Name): | ||
| if default1.name != default2.name: | ||
| match (default1, default2): | ||
| case [nodes.Const(), nodes.Const()]: | ||
| return default1.value != default2.value # type: ignore[no-any-return] | ||
| case [nodes.Name(), nodes.Name()]: | ||
| return default1.name != default2.name # type: ignore[no-any-return] | ||
| case _: |
There was a problem hiding this comment.
A lot better here too !
| if ( | ||
| ( | ||
| isinstance(parent_node, nodes.AnnAssign) | ||
| and parent_node.annotation == current_node | ||
| ) | ||
| or ( | ||
| isinstance(parent_node, nodes.Arguments) | ||
| and current_node | ||
| in ( | ||
| *parent_node.annotations, | ||
| *parent_node.posonlyargs_annotations, | ||
| *parent_node.kwonlyargs_annotations, | ||
| parent_node.varargannotation, | ||
| parent_node.kwargannotation, | ||
| ) | ||
| ) | ||
| or ( | ||
| isinstance(parent_node, nodes.FunctionDef) | ||
| and parent_node.returns == current_node | ||
| ) | ||
| ): | ||
| return True | ||
| match parent_node: | ||
| case nodes.AnnAssign(annotation=ann) if ann == current_node: | ||
| return True | ||
| case nodes.Arguments() if current_node in ( | ||
| *parent_node.annotations, | ||
| *parent_node.posonlyargs_annotations, | ||
| *parent_node.kwonlyargs_annotations, | ||
| parent_node.varargannotation, | ||
| parent_node.kwargannotation, | ||
| ): | ||
| return True | ||
| case nodes.FunctionDef(returns=ret) if ret == current_node: | ||
| return True |
There was a problem hiding this comment.
At some point we need a competition to find the best match uses 😄
There was a problem hiding this comment.
Maybe also an automated updater ? 😄
|
Think I've address all your comments. Let me know if I missed anything.
I've I did my job right, and I'm pretty confident in these, there shouldn't be any behavior changes. At most one or two minor regressions which could be fixed easily. Especially with the improved readability of the match statements. -- |
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
temporarily deployed
to
Backport
GitHub Actions
Inactive
|
Any idea why the last primer results looked so wried? #10528 (comment) It should have been -- -- |
|
The primer are not stable. We don't analyses a particular commit but the latest main, so if the repo changes upstream a lot can changes. Like in home assistant the first consider-using-in comment was moved from line 254 to 258, probably because 4 unrelated line were added somewhere. Also caching in the github action is hard to test. Working on making a better diff the fastest I can (not much, I have claude 200$/month for free for 3 months but haven't been able to prompt it in a way that would result in a gain of productivity, quite the contrary in fact, see #10518). And also working on a particular commit and upgrading from time to time should help tremendously, maybe that's the starting point. |
|
What is really bad is that this expected "new message when the upstream primed repo changes" can hide actual problem. (I'm not reviewing those hundred changes one by one) |
Are you sure it isn't supposed to be stable? AFAICT we do attempt to cache the project repos with a specific commit on main. The primer runs on PRs attempt to restore that cache and reuse the generated messages. I don't think we run pylint twice for each PR change. That's in contrast to the mypy_primer with runs mypy twice (once with the current mypy main and another with the PR head) for each PR change. After looking at the logs for some time now, I might have an idea what went wrong.
So we might be able to fix it if we can checkout a particular commit during the regenerate cache step. I.e. here pylint/.github/workflows/primer_run_pr.yaml Lines 160 to 164 in 90c09a3 pylint/pylint/testutils/_primer/package_to_lint.py Lines 89 to 114 in 90c09a3 |
|
Worth a try, but I think just setting the value in conf with an update script would simplify everything. |
To review I'd suggest to use
Hide whitespace.